Defects the first real-data run exposed, and the fix that unblocks ingest - #651
Merged
Conversation
`_scalar_int` read only the 0-dimensional form, so at the beamline every commanded count came back None: NumPy 2 refuses `int()` on an array that is not 0-d, the reader caught the TypeError, and the shortfall check silently had nothing to compare against. Measured on a real scan file, `/process/acquisition/rotation/num_angles` is a 1-element int32 array, and the flat and dark mode-and-count groups are absent entirely, so `num_angles` was the only commanded fact available and it was the one being dropped. `_scalar_str` already handled exactly this shape, with a comment saying some writers store scalars as 1-element arrays. That is why the timestamp parsed while the integers did not, and why the whole module stayed green: the fixture wrote the 0-d shape the reader could read rather than the shape the producer emits. So the shape is now a fixture parameter, not an assumption. Every `/process/acquisition` scalar can be written either way and the existing matrix runs against both. A longer array stays unreadable rather than being reported as its own first element: that would fabricate a commanded count out of a dataset whose meaning the reader does not know. Found by running the reader against test_005.h5, a real 2-BM scan, after the first scan CORA has ever watched from end to end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
asyncpg hands jsonb back as text unless the connection registers the codecs `postgres.pool` installs. Nothing in the exporter checked, so an export over a plain `asyncpg.connect` produced a bundle whose every payload was one opaque string, wrote it, hashed it, and passed `verify_record_hash.py`. The artifact was perfectly self-consistent about the wrong structure, which is the one failure a record meant to be verifiable by a stranger must not have. Measured, not hypothesised: this is what the first export of the live 2-BM database did. Only redaction noticed, and only by crashing on a `str` where it wanted a mapping, which is luck rather than a check. So the shape is checked where rows enter. A decoded `str` in `payload` or `metadata` can only mean the codec is missing, because both columns are written from a `to_payload()` that returns a dict, and the error says which connection to build instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The table is looked up by `events.event_type`, but the generator keyed it on the class name and a comment asserted those were the same thing. They are not: the class is `ActorRegistered` and the append path writes `"ActorRegisteredV2"`. Redaction refuses an event type it has no entry for, and every database holds an Actor registration from bootstrap, so the published-export path could not succeed against ANY real record. A legacy record would have exported fine, since the V1 string was the one in the table. CI could not see it. The drift test compares the generator against its own committed output, so a generator deriving the wrong key yields a wrong table and a green test. It took running an export against the live 2-BM database to surface it. The generator now asks each module's own `event_type_name`, the same function the append path calls, so the two cannot drift by construction. Those functions discriminate on type, so an instance that was never `__init__`ed answers correctly; one that reads a field aborts the run rather than falling back to the class name. The new fitness test deliberately does NOT reuse that route. It reads the string literals those functions return, statically, without importing anything or running the generator, because a guard sharing the generator's mechanism would share its blind spot. Its second test is a canary on the scanner: a scan that silently matched nothing would pass forever. One consequence stated rather than buried: the V1 string is no longer in the table, so a pre-vault database carrying legacy `ActorRegistered` rows now fails closed on export. That is the safe direction, since a V1 payload carries the `name` field the PII vault removed and publishing it needs a deliberate disposition rather than an inherited one, but it is a behaviour change and retired wire names have no home in a generated table yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
2-BM's scan files carry two, and the obvious one is wrong. Measured across six consecutive files, every file's `start_date` equals the PREVIOUS file's `end_date`, while every `end_date` falls within five seconds of its own close. The areaDetector timestamp attribute refreshes only while frames flow, so a file opened for scan N carries the value left from scan N-1's last frame. The timestamp PV is healthy: read live it returns the correct instant. The reader only ever read `start_date`, and the ingest policy is that a parseable file timestamp beats an operator's and supplying both is refused as ambiguous. Together those would have recorded a scan watched from end to end this morning as captured the previous evening, with no way to correct it. That is a false fact in a record whose whole purpose is to be true, so ingest at 2-BM stayed blocked rather than writing it. Which timestamp to believe is a deployment fact, not a layout fact, so the reader is told and reads what it is told. The default is unchanged, `start_date`, so no other deployment moves. There is no fallback: a deployment that declared `end_date` and got a file without one gets a refusal, because silently reading the other timestamp would hand back exactly the value the declaration exists to avoid. `Description.start_date` becomes `captured_at`, since the field no longer always holds a start date, and gains `captured_at_source` so the record states which fact it used instead of leaving a reader to assume. The policy itself is deliberately unchanged: a file value still beats an operator's, because the fix for a bad writer is to declare the good timestamp, not to let every caller assert over the file. The descriptor records the choice for humans and the setting drives the runtime, since nothing reads the descriptor at runtime yet. They agree by hand for now, which the descriptor docstring says out loud. Naming reviewed: `_CAPTURED_AT_SOURCES` made private to match every sibling constant in the module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The diff-coverage gate counts refusal paths, and reworking `_scalar_int` added three it had no fixture for: a count stored as text, a count stored as text inside a one-element array, and a value that is neither a number nor a sized thing. Worth testing rather than waving through. The first two are the same decode the timestamp path already does, so a count the reader could have read but silently dropped would be this commit's parent bug in a different costume. The third pins the never-raise contract: an unreadable count reports unknowable rather than escaping the worker thread as a failed describe. Real HDF5 files rather than mocks, since the whole lesson of the day is that a fixture built to match the reader proves nothing about a file built by a writer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three defects the first real-data run exposed, each one invisible to a green CI. Every fix is proven to fail without it.
They share a root cause worth naming: fixtures and guards were built from docstrings and from the code's own output, so nothing compared them against what production actually writes.
Read the commanded counts 2-BM actually writes (
713171618c)_scalar_inthandled only 0-dimensional datasets. 2-BM writes/process/acquisition/rotation/num_anglesas a 1-elementint32array, NumPy 2 refusesint()on those, and the reader turned theTypeErrorintoNone. So every commanded count wasNoneat the beamline and the shortfall check had nothing to compare against, which also makes DATA-8 unanswerable from the record._scalar_stralready handled this exact shape, which is why the timestamp parsed and the integers did not. The scalar shape is now a fixture parameter rather than an assumption, so the existing matrix runs against both forms.Refuse to export a record whose payloads are strings (
3a07f5da0f)asyncpg returns jsonb as text unless the connection registers the codecs
postgres.poolinstalls. Nothing checked. The first export of the live 2-BM database produced a bundle whose every payload was one opaque string, wrote it, hashed it, and passedverify_record_hash.py— perfectly self-consistent about the wrong structure, which is the one failure a record meant to be verifiable by a stranger must not have. Only redaction noticed, by crashing on astrwhere it wanted a mapping.Key the redaction table on what events are stored under (
ae39e5299e)The table is looked up by
events.event_typebut was keyed on the class name, with a comment asserting those are the same. The class isActorRegistered; the append path writes"ActorRegisteredV2". Redaction refuses an unknown event type and every database holds an Actor registration from bootstrap, so the published-export path could not succeed against any real record. A legacy record would have exported fine, since the V1 string was the one in the table.The drift test compares the generator against its own committed output, so a generator deriving the wrong key gives a wrong table and a green test. The generator now asks each module's own
event_type_name, and the new fitness test reaches the same answer by a deliberately different route (static literal scan, no imports, no generator), because a guard sharing the generator's mechanism shares its blind spot.One behaviour change, stated rather than buried: the V1 string is no longer in the table, so a pre-vault database carrying legacy
ActorRegisteredrows now fails closed on export. That is the safe direction, since a V1 payload carries thenamefield the PII vault removed, but retired wire names have no home in a generated table yet.Let a deployment say which timestamp is the acquisition time (
b1c51735df)2-BM's scan files carry two timestamps and the obvious one is wrong. Measured across six consecutive files, every file's
start_dateequals the previous file'send_date, while everyend_datefalls within five seconds of its own close. The areaDetector timestamp attribute refreshes only while frames flow, so a file opened for scan N carries the value left from scan N-1's last frame. The timestamp PV itself is healthy; read live it returns the correct instant.The reader only ever read
start_date, and the ingest policy is that a parseable file timestamp beats an operator's, with both supplied being refused as ambiguous. Together those would have recorded a scan we watched from end to end as captured the previous evening, uncorrectably. So ingest at 2-BM stayed blocked rather than writing a false fact into the record.Which timestamp to believe is a deployment fact, not a layout fact. The reader is now told, and reads what it is told:
start_date), so no other deployment moves.end_dateand getting a file without one is a refusal, since silently reading the other timestamp returns exactly the value the declaration exists to avoid.Description.start_datebecomescaptured_at(the field no longer always holds a start date) and gainscaptured_at_source, so the record states which fact it used.The descriptor records the choice for humans, the setting drives the runtime, and they agree by hand until the descriptor is read at runtime, which the descriptor docstring says out loud.
Naming reviewed by
naming-r3-reviewer:captured_at_source, thescan_prefix, and the descriptor key were confirmed against R1-R6, and its one finding (make the lookup constant private) is applied.Not in this PR
The retrospective gate review of #650 raised further P0s on the published-artifact path (manifest not bound to the written record, raw Run UUIDs in the published manifest, tier-2 clearances describing a different producer than the Conductor,
is_simulatedinverted). Those are separate slices and will carry the full panel.🤖 Generated with Claude Code